test/extended/node/node_e2e/node.go (4)

282-432: ⚡ Quick win

Three near-identical pod specs — extract a builder helper.

The three pods (liveness-probe, startup-probe, liveness-probe-no-term) duplicate ~50 lines of spec each (same image, security context, command, ports). Only the probe kind, name, and probe-level TerminationGracePeriodSeconds differ. A small builder would shrink the test by ~100 lines and make the differences explicit.

♻️ Sketch

buildProbePod := func(name string, probe *corev1.Probe, kind string) *corev1.Pod {
    container := corev1.Container{
        Name:    "test",
        Image:   "quay.io/openshifttest/nginx-alpine@sha256:04f316442d48ba60e3ea0b5a67eb89b0b667abf1c198a3d0056ca748736336a0",
        Command: []string{"bash", "-c", "sleep 100000000"},
        Ports:   []corev1.ContainerPort{{ContainerPort: 8080}},
        SecurityContext: &corev1.SecurityContext{
            AllowPrivilegeEscalation: ptr.To(false),
            Capabilities:             &corev1.Capabilities{Drop: []corev1.Capability{"ALL"}},
        },
    }
    switch kind {
    case "liveness":
        container.LivenessProbe = probe
    case "startup":
        container.StartupProbe = probe
    }
    return &corev1.Pod{
        ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
        Spec: corev1.PodSpec{
            TerminationGracePeriodSeconds: ptr.To[int64](60),
            SecurityContext: &corev1.PodSecurityContext{
                RunAsNonRoot:   ptr.To(true),
                SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault},
            },
            Containers: []corev1.Container{container},
        },
    }
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/extended/node/node_e2e/node.go` around lines 282 - 432, The three pod
specs (liveness-probe, startup-probe, liveness-probe-no-term) duplicate
container/image/security/command/ports setup — extract a builder like
buildProbePod(name string, probe *corev1.Probe, kind string) that constructs the
common corev1.Container (Image, Command, Ports, SecurityContext) and attaches
either LivenessProbe or StartupProbe based on kind, and returns the full
*corev1.Pod with the shared PodSpec (pod-level TerminationGracePeriodSeconds and
PodSecurityContext); replace the three inline pod literals with calls to
buildProbePod("liveness-probe", livenessProbe, "liveness"),
buildProbePod("startup-probe", startupProbe, "startup") and
buildProbePod("liveness-probe-no-term", nil, "liveness") and keep
verifyProbeTermination calls unchanged.

---

216-279: 🏗️ Heavy lift

Use the Events API directly instead of parsing humanized oc describe output.

The helper extracts timestamps by splitting describe output and indexing fields[2]. This is fragile:

• The "Age" column position depends on the describe template and event aggregation. While aggregated events (e.g., 60s (x3 over 90s)) keep the last-seen value at fields[2], any format change breaks parsing silently.
• The substring match "Started container" and multi-substring match for probe failures could match unrelated events if more containers are added.

Use the Events API directly: query oc.KubeClient().CoreV1().Events(namespace).List(...), filter by involvedObject and reason (Started, Killing), and consume event.FirstTimestamp / event.LastTimestamp directly without parsing humanized durations. This pattern is idiomatic across the test suite.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/extended/node/node_e2e/node.go` around lines 216 - 279, The current
verifyProbeTermination function is brittle because it parses humanized `oc
describe` output and indexes fields[2]; replace that logic with a direct Events
API query: inside verifyProbeTermination (and remove dependence on
parseDurationToSeconds), call
oc.KubeClient().CoreV1().Events(namespace).List(...) (or use the client wrapper
available in the test helpers), filter events by event.InvolvedObject.Name ==
podName and by event.Reason == "Started" for container starts and event.Reason
== "Killing" (or the probe-failure reason used by your cluster) for
probe/termination events, then use the event.FirstTimestamp/LastTimestamp fields
to compute seconds difference and compare to expectedTerminationSec with the
same tolerance logic; log the selected event timestamps and keep the
polling/wait.PollUntilContextTimeout wrapper and return true when the time diff
is within range.

---

185-213: ⚡ Quick win

Replace custom duration parser with time.ParseDuration.

The Go standard library already parses the exact formats kubectl emits ("45s", "1m30s", "1h2m3s"). The current implementation silently returns 0, nil for unrecognized inputs (e.g., "5h" since it matches neither the "m" nor "s" branches), which would cause downstream timing arithmetic to be wrong without surfacing an error. Switching to time.ParseDuration removes the custom code path and the silent-zero failure mode.

♻️ Proposed refactor

-		// Helper function to parse duration string like "1m30s" or "45s" to seconds
-		parseDurationToSeconds := func(durationStr string) (int, error) {
-			var totalSeconds int
-			if strings.Contains(durationStr, "m") {
-				parts := strings.Split(durationStr, "m")
-				minutes, err := strconv.Atoi(parts[0])
-				if err != nil {
-					return 0, err
-				}
-				totalSeconds = minutes * 60
-				if len(parts) > 1 && strings.Contains(parts[1], "s") {
-					secStr := strings.TrimSuffix(parts[1], "s")
-					if secStr != "" {
-						seconds, err := strconv.Atoi(secStr)
-						if err != nil {
-							return 0, err
-						}
-						totalSeconds += seconds
-					}
-				}
-			} else if strings.Contains(durationStr, "s") {
-				secStr := strings.TrimSuffix(durationStr, "s")
-				seconds, err := strconv.Atoi(secStr)
-				if err != nil {
-					return 0, err
-				}
-				totalSeconds = seconds
-			}
-			return totalSeconds, nil
-		}
+		// Helper function to parse duration string like "1m30s" or "45s" to seconds
+		parseDurationToSeconds := func(durationStr string) (int, error) {
+			d, err := time.ParseDuration(durationStr)
+			if err != nil {
+				return 0, err
+			}
+			return int(d.Seconds()), nil
+		}

The strconv import can then be removed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/extended/node/node_e2e/node.go` around lines 185 - 213, The custom
parser function parseDurationToSeconds should be replaced to use
time.ParseDuration: call time.ParseDuration(durationStr), return
int(duration.Seconds()) on success and propagate the error on failure so
unrecognized inputs (e.g., "5h") don't silently return 0; update the function
signature/returns accordingly, remove the now-unused strconv import, and ensure
callers still get an int number of seconds from the parsed duration.

---

288-291: 💤 Low value

Use a pointer helper instead of &[]T{v}[0].

The &[]int64{60}[0] / &[]bool{true}[0] pattern (used 11 times in this test) allocates a single-element slice solely to take its address. Prefer ptr.To from k8s.io/utils/ptr for clarity and consistency with the rest of the k8s ecosystem.

♻️ Example refactor

 import (
   ...
+  "k8s.io/utils/ptr"
   ...
 )

-				TerminationGracePeriodSeconds: &[]int64{60}[0],
+				TerminationGracePeriodSeconds: ptr.To[int64](60),
 				SecurityContext: &corev1.PodSecurityContext{
-					RunAsNonRoot: &[]bool{true}[0],
+					RunAsNonRoot: ptr.To(true),
 					...
 				},
 				...
-							AllowPrivilegeEscalation: &[]bool{false}[0],
+							AllowPrivilegeEscalation: ptr.To(false),
 							...
-							TerminationGracePeriodSeconds: &[]int64{10}[0],
+							TerminationGracePeriodSeconds: ptr.To[int64](10),

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/extended/node/node_e2e/node.go` around lines 288 - 291, Replace the
hacky &[]T{v}[0] pointer constructions with the ptr.To helper from
k8s.io/utils/ptr for clarity and consistency: e.g. change
TerminationGracePeriodSeconds: &[]int64{60}[0] to use ptr.To(int64(60)) and
RunAsNonRoot: &[]bool{true}[0] to ptr.To(true); update all other similar
occurrences (about 11 spots) in this file (look for
TerminationGracePeriodSeconds, RunAsNonRoot, SeccompProfile usages) and add the
import for "k8s.io/utils/ptr" if not already present.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes